refactor(billing): security_invoker views + RLS on base tables#709
Conversation
Convert grida_billing's API surface to the canonical Supabase pattern: security_invoker=true views over RLS-protected base tables, replacing SECURITY DEFINER views over a fully-locked schema. Functionally equivalent for every legitimate consumer; satisfies splinter lint 0010 (security_definer_view) which the Dashboard's Database Advisors flagged. What changed: - New migration 20260507223000_grida_billing_security_invoker.sql. The pushed 20260506132900 stays untouched. - grida_billing schema now grants USAGE to authenticated + service_role (security_invoker views resolve `grida_billing.*` as the caller). - account / subscription / audit drop their RESTRICTIVE deny-all policies and gain SELECT-only permissive policies (member_can_select for the first two, owner_can_select for audit). Writes stay service-role-only. - product_catalogue / stripe_event remain fully locked — no API consumer reads them. - v_billing_subscription / v_billing_audit flip to security_invoker=true. Their auth-side WHERE clauses move to the base-table policies; the views keep only the business filter (`status <> 'canceled'`). - Both views explicitly REVOKE from PUBLIC, anon, authenticated and re-GRANT to authenticated + service_role. Closes Supabase's default ALL-on-public privilege creep that was making anon implicitly able to discover them via GraphQL introspection. Behavioral equivalence (verified): - Authenticated org members see the same rows pre/post (auth filter just moved from view body to RLS policy). - Random users with no org → 0 rows, both before and after. - Owners read v_billing_audit; non-owner members blocked, both before and after. - anon: previously "implicit SELECT, returns 0 rows via NULL auth.uid()" → now "no SELECT grant, returns permission_denied". Strictly better posture; no legitimate flow affected. Tests: - pgTAP §13 (RLS denial contract) rewritten for the new model: 10 anon-denial assertions + 3 authenticated-non-member-sees-zero + 2 locked-table denials + 5 INSERT denials = 20 total. Plan stays at 57; all tests pass. - E2E billing suite (lifecycle, idempotency, tampered-signature) unchanged — webhook projector path untouched. - pnpm --filter editor typecheck: clean. - pnpm lint: 0 errors. - 476 editor unit tests: pass. Tooling: - New `just lint-supabase-db` recipe runs Splinter advisors against the local DB. CLI doesn't ship a native `supabase db check` yet (supabase/cli#3839 open); inline curl + psql + awk suffices. Recipe links the upstream tracking issue + our comment so future maintainers can drop it when the CLI ships native support. Sibling reading: - supabase/cli#3839 — feature request for first-class CLI advisor command (we left a comment with our use case + recipe at #issuecomment-4397668907)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe PR refactors the grida_billing schema security model from view-based row filtering to table-level RLS with security_invoker views, updates associated tests, tightens schema/function grants, and adds a just recipe to run Supabase splinter advisor locally. ChangesBilling Schema Security Refactoring
Database Linting Tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)justfileMicrosoft Presidio Analyzer failed to scan this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 393588a508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| -- ─── 1. Schema USAGE ────────────────────────────────────────────── | ||
| REVOKE ALL ON SCHEMA grida_billing FROM PUBLIC; | ||
| GRANT USAGE ON SCHEMA grida_billing TO authenticated, service_role; |
There was a problem hiding this comment.
Revoke internal function execute rights before schema access
Granting authenticated USAGE on grida_billing makes every routine in that schema callable by authenticated users unless EXECUTE is explicitly revoked, and the existing billing migration defines multiple SECURITY DEFINER functions in grida_billing without corresponding REVOKE ... ON FUNCTION grida_billing.* FROM PUBLIC (only the public.fn_billing_* wrappers are locked down). This opens a privilege-escalation path where any signed-in user can invoke internal mutating functions (for example, event projector/customer attach paths) directly and bypass the intended service-role-only boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
justfile (1)
36-37: ⚡ Quick win
levelsvalue is interpolated as an awk regex without sanitization.
{{replace(levels, ",", "|")}}is dropped verbatim into an awk ERE. Passinglevels="WARN,ERROR|INFO"or any other ERE metacharacter (.,*,[,() would silently widen or break the match. For a dev-only tool the blast radius is small, but a simple anchored string-comparison is safer and equally readable:🔧 Proposed hardening (pure string match)
- | awk -F'\t' -v lvls="^({{replace(levels, ",", "|")}})$" \ - 'NF >= 7 && $3 ~ lvls { print " [" $3 "] " $1 ": " $7 }' \ + | awk -F'\t' -v lvls=",{{levels}}," \ + 'NF >= 7 && index(lvls, "," $3 ",") { print " [" $3 "] " $1 ": " $7 }' \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@justfile` around lines 36 - 37, The awk invocation interpolates {{replace(levels, ",", "|")}} into lvls and treats it as a regular expression, allowing metacharacters in levels to widen or break matching; change to a pure string comparison by escaping/sanitizing levels before interpolation or — better — avoid EREs entirely and implement a string-equality check in the awk script (e.g., split the levels string on commas and compare $3 against each element) so lvls no longer contains raw regex metacharacters; update the command that defines lvls and the awk condition that uses $3 to perform exact string matches instead of $3 ~ lvls.supabase/tests/test_grida_billing_test.sql (1)
453-470: ⚡ Quick winSection 13 is missing positive-read assertions for the newly-opened tables.
The section thoroughly tests negative cases (0-row RLS filter for non-members,
42501for locked tables and INSERTs). However, per the coding guideline, tests for tables that alter RLS should include both positive and negative assertions. There's no test confirming that an authenticated org member (e.g.,alice) can successfullySELECTfromgrida_billing.account,grida_billing.subscription, andgrida_billing.auditdirectly (the existing positive coverage in sections 9/10 is only through thepublicviews).✅ Suggested additions to section 13 (and update
SELECT plan(N)accordingly)-- ── authenticated org owner: positive direct-table reads ─────────────── SET LOCAL ROLE authenticated; SELECT set_config('request.jwt.claim.sub', current_setting('test.alice_uid'), true); SELECT ok( (SELECT count(*) FROM grida_billing.account WHERE organization_id = 2) > 0, 'alice (owner) sees her org account row directly via RLS' ); SELECT ok( (SELECT count(*) FROM grida_billing.subscription WHERE organization_id = 2) > 0, 'alice (owner) sees her org subscription row directly via RLS' ); SELECT ok( (SELECT count(*) FROM grida_billing.audit WHERE organization_id = 2) > 0, 'alice (owner) sees her org audit row directly via RLS' ); RESET ROLE;As per coding guidelines: "Include both positive and negative assertions (ok(...), is(...), throws_ok(...))" and "Add pgTAP test coverage for every table that alters RLS, permissions, or tenant boundaries, including at minimum read isolation tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/tests/test_grida_billing_test.sql` around lines 453 - 470, Add positive read assertions for authenticated org members by setting the JWT sub to an owner (use SET LOCAL ROLE authenticated; SELECT set_config('request.jwt.claim.sub', current_setting('test.alice_uid'), true);), then add ok(...) tests that SELECT count(*) FROM grida_billing.account WHERE organization_id = 2 > 0, from grida_billing.subscription WHERE organization_id = 2 > 0, and from grida_billing.audit WHERE organization_id = 2 > 0 to confirm alice can directly read those tables via RLS, then RESET ROLE; ensure tests use ok(...) and update the SELECT plan(N) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@justfile`:
- Around line 34-38: The recipe currently pipes psql into awk and sort so psql
failures are ignored; change it to capture psql output to a temporary file (use
mktemp), run the psql invocation (the `@psql` ... -f /tmp/splinter.sql ...
command) into that temp file, check its exit status and fail the recipe if
non‑zero, and only then run the awk ('awk -F'\t' -v lvls=... ...') and sort -u
steps reading from the temp file; ensure the temp file is removed on exit. This
preserves the existing awk/filter logic but guarantees psql errors cause the
recipe to fail.
- Line 33: The curl invocation that writes to the fixed path `/tmp/splinter.sql`
should be changed to download to a unique temporary file, verify its integrity,
and clean it up; update the URL to pin a commit SHA instead of `main`, create a
temp file (e.g., via mktemp), download there (the existing `curl -sSfL` usage),
compute and compare a provided checksum (sha256) before executing, and ensure
removal of the temp file on exit/failure (use a trap) so parallel CI jobs don’t
clobber each other and the SQL is integrity-checked.
In `@supabase/schemas/grida_billing.sql`:
- Around line 60-67: The RLS policies (e.g., member_can_select and
owner_can_select) reference public table join columns without indexes, causing
sequential scans during policy evaluation; add indexes for
public.organization_member.user_id and public.organization.owner_id (create
indexes named organization_member_user_id_idx and organization_owner_id_idx
respectively) so policy subqueries used by the account/subscription/audit
policies can use index lookups instead of sequential scans; ensure to use IF NOT
EXISTS semantics when adding these indexes so repeated migrations are safe.
---
Nitpick comments:
In `@justfile`:
- Around line 36-37: The awk invocation interpolates {{replace(levels, ",",
"|")}} into lvls and treats it as a regular expression, allowing metacharacters
in levels to widen or break matching; change to a pure string comparison by
escaping/sanitizing levels before interpolation or — better — avoid EREs
entirely and implement a string-equality check in the awk script (e.g., split
the levels string on commas and compare $3 against each element) so lvls no
longer contains raw regex metacharacters; update the command that defines lvls
and the awk condition that uses $3 to perform exact string matches instead of $3
~ lvls.
In `@supabase/tests/test_grida_billing_test.sql`:
- Around line 453-470: Add positive read assertions for authenticated org
members by setting the JWT sub to an owner (use SET LOCAL ROLE authenticated;
SELECT set_config('request.jwt.claim.sub', current_setting('test.alice_uid'),
true);), then add ok(...) tests that SELECT count(*) FROM grida_billing.account
WHERE organization_id = 2 > 0, from grida_billing.subscription WHERE
organization_id = 2 > 0, and from grida_billing.audit WHERE organization_id = 2
> 0 to confirm alice can directly read those tables via RLS, then RESET ROLE;
ensure tests use ok(...) and update the SELECT plan(N) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 190f513c-5b97-4f92-a6b9-cbe523b53c16
📒 Files selected for processing (4)
justfilesupabase/migrations/20260507223000_grida_billing_security_invoker.sqlsupabase/schemas/grida_billing.sqlsupabase/tests/test_grida_billing_test.sql
| CREATE POLICY member_can_select ON grida_billing.account | ||
| FOR SELECT TO authenticated | ||
| USING ( | ||
| organization_id IN ( | ||
| SELECT om.organization_id FROM public.organization_member om | ||
| WHERE om.user_id = (SELECT auth.uid()) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify indexes exist on public.organization_member(user_id) and
# public.organization(owner_id) — the columns referenced in the new RLS policies.
rg -n --type sql -i 'create.*index.*organization_member.*user_id\|create.*index.*user_id.*organization_member' \
|| echo "No index on organization_member.user_id found"
rg -n --type sql -i 'create.*index.*organization.*owner_id\|create.*index.*owner_id.*organization[^_]' \
|| echo "No index on organization.owner_id found"Repository: gridaco/grida
Length of output: 143
Add indexes on RLS join-key columns to avoid sequential scans during policy evaluation.
All three new SELECT policies subquery public tables without indexes on their join conditions:
| Policy | Subquery join column | Status |
|---|---|---|
member_can_select (account/subscription) |
public.organization_member.user_id |
Missing |
owner_can_select (audit) |
public.organization.owner_id |
Missing |
Each RLS-gated row scan will trigger a sequential table scan, once per candidate row. Under load this becomes a hot path.
Add these indexes to the public schema:
SQL Fix
CREATE INDEX IF NOT EXISTS organization_member_user_id_idx
ON public.organization_member (user_id);
CREATE INDEX IF NOT EXISTS organization_owner_id_idx
ON public.organization (owner_id);Applies to lines: 60-67, 119-126, 261-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/schemas/grida_billing.sql` around lines 60 - 67, The RLS policies
(e.g., member_can_select and owner_can_select) reference public table join
columns without indexes, causing sequential scans during policy evaluation; add
indexes for public.organization_member.user_id and public.organization.owner_id
(create indexes named organization_member_user_id_idx and
organization_owner_id_idx respectively) so policy subqueries used by the
account/subscription/audit policies can use index lookups instead of sequential
scans; ensure to use IF NOT EXISTS semantics when adding these indexes so
repeated migrations are safe.
PR review fixes from #709. Directly amends the unmerged Pattern A migration (`20260507223000_grida_billing_security_invoker.sql`) so production gets one clean migration instead of a delta-on-delta. Codex P1 — privilege escalation closed: Granting USAGE on `grida_billing` to authenticated (required for security_invoker views to resolve `grida_billing.*` as the caller) also made every internal SECURITY DEFINER routine reachable, because EXECUTE on functions defaults to PUBLIC and authenticated inherits PUBLIC. The original ALTER DEFAULT PRIVILEGES line only revoked from authenticated/anon, not PUBLIC, so authenticated could call the webhook projector (`fn_apply_stripe_event`) directly with arbitrary payloads and bypass signature verification. Fix: explicit `REVOKE EXECUTE ON ALL FUNCTIONS IN SCHEMA grida_billing FROM PUBLIC, anon, authenticated` for existing routines, and `ALTER DEFAULT PRIVILEGES … REVOKE EXECUTE … FROM PUBLIC` for future ones. Verified post-migration: only `postgres` and `service_role` retain EXECUTE on every grida_billing.fn_* and tg_*. CodeRabbit #4 (partial) — RLS join-key index: `owner_can_select` on grida_billing.audit subqueries `public.organization` by `owner_id`, which has no index. Without it, every RLS evaluation on audit reads triggers a seq scan. Cheap fix. (CodeRabbit also flagged `organization_member.user_id`, but that index already exists — verified.) CodeRabbit #2 — splinter download race + integrity: `/tmp/splinter.sql` collides under parallel CI matrix runs. Switched to `mktemp -t splinter.XXXXXX.sql` with `trap … EXIT` cleanup so concurrent invocations get isolated paths. CodeRabbit #3 — pipeline error swallowing: `just` runs `/bin/sh` without pipefail; a `psql … | awk … | sort` failure used to exit 0 with empty output (no signal that linting never ran). Switched the recipe to a bash shebang with `set -euo pipefail` and capture-then-pipe, so connection/auth/schema-drift failures propagate. Verified: a bogus connection URL now exits non-zero with a clear psql error. Verification: - `supabase db reset` → both migrations apply clean - `supabase test db` → 135 pgTAP tests pass - `pnpm --filter editor typecheck` → clean - `pnpm lint` → 0 errors - `just lint-supabase-db` → no `security_definer_view` findings; the only remaining warnings are by-design GraphQL-discoverability notices on the two billing views (out of scope, see PR description) - `pg_indexes` confirms `organization_owner_id_idx` is present - `information_schema.routine_privileges` confirms only postgres + service_role hold EXECUTE on every grida_billing.fn_* / tg_* E2E suite not re-run (requires `next dev` on :3000, which the sandbox correctly refuses to autostart). Zero TS/projector code touched in this commit, so the suite's pass/fail is unchanged from prior cycle.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
justfile (1)
40-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
splinter.sqlto an immutable revision (and ideally verify checksum).Fetching executable SQL from
mainmakes lint results non-reproducible and weakens supply-chain trust. Prefer a commit-SHA URL and optional SHA-256 verification beforepsql.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@justfile` at line 40, Replace the unstable curl download of splinter.sql from the branch with a pinned commit URL and add checksum verification: update the curl target that writes to the $splinter variable to fetch the file using a commit-SHA URL (not the /main path) and then verify its SHA-256 matches an expected value before proceeding (e.g. compute the downloaded file's sha256 and fail if it doesn't match); reference the existing $splinter variable and the splinter.sql download step so the Justfile target aborts on checksum mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@justfile`:
- Line 43: The psql invocation passes the DSN/template variable unquoted which
can break for DSNs with spaces or shell-sensitive chars; update the command that
builds output (the psql call that currently uses {{url}}) to wrap the URL in
double quotes ("{{url}}") so the shell treats it as a single safe argument to
psql; ensure the change is applied where the output=$(psql {{url}} -X -qtA
-F$'\t' --pset=pager=off \) command is defined.
---
Duplicate comments:
In `@justfile`:
- Line 40: Replace the unstable curl download of splinter.sql from the branch
with a pinned commit URL and add checksum verification: update the curl target
that writes to the $splinter variable to fetch the file using a commit-SHA URL
(not the /main path) and then verify its SHA-256 matches an expected value
before proceeding (e.g. compute the downloaded file's sha256 and fail if it
doesn't match); reference the existing $splinter variable and the splinter.sql
download step so the Justfile target aborts on checksum mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0ebaac7-17f2-40fa-bd8d-5f8059fcc51e
📒 Files selected for processing (3)
justfilesupabase/migrations/20260507223000_grida_billing_security_invoker.sqlsupabase/schemas/grida_billing.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- supabase/schemas/grida_billing.sql
- supabase/migrations/20260507223000_grida_billing_security_invoker.sql
| curl -sSfL https://raw.githubusercontent.com/supabase/splinter/main/splinter.sql -o "$splinter" | ||
| # Capture before filtering so any psql error propagates instead of being | ||
| # masked by an empty result set. | ||
| output=$(psql {{url}} -X -qtA -F$'\t' --pset=pager=off \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Verify current interpolation is unquoted in recipe
rg -n 'output=\(psql \{\{url\}\}' justfileRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Read the justfile around line 43 to see the context
sed -n '40,50p' justfileRepository: gridaco/grida
Length of output: 585
Quote the url argument in the psql call.
Line 43 currently passes {{url}} unquoted to psql. Change to "{{url}}" to safely handle DSNs containing shell-significant characters (spaces, wildcards, dollar signs, etc.).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@justfile` at line 43, The psql invocation passes the DSN/template variable
unquoted which can break for DSNs with spaces or shell-sensitive chars; update
the command that builds output (the psql call that currently uses {{url}}) to
wrap the URL in double quotes ("{{url}}") so the shell treats it as a single
safe argument to psql; ensure the change is applied where the output=$(psql
{{url}} -X -qtA -F$'\t' --pset=pager=off \) command is defined.
Summary
Converts
grida_billing's API surface fromSECURITY DEFINERviews over a locked-down schema →security_invoker = trueviews over RLS-protected base tables. Same row sets for every legitimate consumer; silences splinter lint 0010 (security_definer_view) that the Dashboard's Database Advisors page was flagging.This is the canonical Supabase pattern — protection lives in table-level RLS policies that the rest of the codebase already audits the same way, instead of in a SECURITY DEFINER view body that's easy to read past.
What landed
New migration supabase/migrations/20260507223000_grida_billing_security_invoker.sql. The pushed
20260506132900_grida_billing.sqlis unchanged.grida_billingschemaauthenticated+service_role; revoked from PUBLIC. Required because security_invoker views resolvegrida_billing.*as the caller.account/subscription/auditmember_can_select/owner_can_select) + table-level GRANT SELECT to authenticated. Writes still service-role only.product_catalogue/stripe_eventv_billing_subscriptionsecurity_invoker = false, WHERE filter onauth.uid()security_invoker = true, body keeps onlyWHERE status <> 'canceled'(auth scoping moved to RLS)v_billing_auditREVOKE FROM PUBLIC, anon, authenticatedthenGRANT SELECT TO authenticated, service_roleBehavioral equivalence
Verified — see test plan. Quick summary:
v_billing_audit: same rows; non-owner members still blocked.anonquerying the views directly: previously got an empty array viaauth.uid() = NULLfilter; now getspermission_deniedbecause we explicitly revoked. Strictly better posture; no legitimate flow hits this path (all 4 callsites use user-authedcreateClient()).Tests
supabase/tests/test_grida_billing_test.sql§13 RLS contract rewritten for the new model:product_catalogue,stripe_event)pnpm --filter editor typecheckpnpm lint(oxlint)supabase db reset(both migrations apply)supabase test db(135 pgTAP, +20 net)BILLING_E2E=1suite (real Stripe sandbox)just lint-supabase-dbsecurity_definer_viewfindingsTooling
New
just lint-supabase-dbrecipe runs Supabase Splinter advisors against the local DB — same lints the Dashboard's Database Advisors page shows. The CLI doesn't yet ship a native command for this; inlinecurl + psql + awkis enough.Recipe links the upstream tracking issue and our comment, so future maintainers can drop it when the CLI ships native support: supabase/cli#3839 (our comment).
Test plan
supabase db resetagainst a fresh local supabasesupabase test db— 135 tests pass, including the rewritten §13BILLING_E2E=1 pnpm --filter editor exec vitest run lib/billing/__tests__/e2e— 3/3 passjust lint-supabase-dband confirm nosecurity_definer_view(lint 0010) findings remain/organizations/<org>/settings/billingas an authenticated org member — same data shows as beforev_billing_*warnings are goneOut of scope / known noise
pg_graphql_authenticated_table_exposedlint (different from 0010) still fires for the two views — by design. The views ARE intentionally discoverable by authenticated users; RLS filters which rows they see. Same lint fires for ~20 other public tables (organization, project, document, etc.) — repo-wide pattern, separate concern.grida_forms, public bucket listing, etc.) are out of scope and predate billing.Summary by CodeRabbit
New Features
Chores